feat(evaluator-sdk): sandboxed containerized Fabric runner (AALGO-321)#604
Open
SandyChapman wants to merge 1 commit into
Open
feat(evaluator-sdk): sandboxed containerized Fabric runner (AALGO-321)#604SandyChapman wants to merge 1 commit into
SandyChapman wants to merge 1 commit into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a provider-neutral sandbox API, Docker implementation, Fabric image builder, containerized Fabric evaluation runtime, shared trial/evidence helpers, an end-to-end example, tests, and ChangesFabric container evaluation
Relay dependency and metadata
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/api.py (1)
120-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
import tempfileto module top-level.Local import inside
_write_fileis unconventional; hoist to the top of the file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/api.py` around lines 120 - 127, The `_write_file` helper currently does a local `import tempfile`, which should be hoisted to the module top level for consistency. Move the import alongside the other top-level imports in this sandbox API module and remove the inline import from `_write_file`, keeping the rest of the function behavior unchanged.packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/providers/docker.py (2)
92-121: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo handling for a missing
dockerbinary.
_runonly catches timeouts; ifdocker_binisn't found,asyncio.create_subprocess_execraises a rawFileNotFoundErrorthat propagates uncaught through every operation (create,exec,status,close) with no actionable message.🛡️ Proposed fix
- proc = await asyncio.create_subprocess_exec( - *argv, - stdin=asyncio.subprocess.PIPE if stdin is not None else None, - stdout=asyncio.subprocess.PIPE, - stderr=asyncio.subprocess.PIPE, - start_new_session=True, - ) + try: + proc = await asyncio.create_subprocess_exec( + *argv, + stdin=asyncio.subprocess.PIPE if stdin is not None else None, + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + start_new_session=True, + ) + except FileNotFoundError as exc: + raise RuntimeError(f"docker binary not found: {argv[0]!r}") from exc🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/providers/docker.py` around lines 92 - 121, The _run chokepoint in DockerProvider only handles timeouts, so a missing docker binary still bubbles up as a raw FileNotFoundError through create, exec, status, and close. Update _run to catch the create_subprocess_exec failure for the docker executable and re-raise a clearer, actionable error that names the missing binary and the argv/context, while preserving the existing timeout handling and return semantics for successful runs.
45-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused
_KEEP_ALIVE_COMMANDconstant duplicatescreate()'s hardcoded argv.
create()hardcodes["sh", "-c", "exec sleep infinity"]directly instead of using this constant, so they can silently drift.♻️ Consolidate
-argv += [spec.image, "sh", "-c", "exec sleep infinity"] +argv += [spec.image, *shlex.split(_KEEP_ALIVE_COMMAND)]Also applies to: 137-139
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/providers/docker.py` at line 45, The keep-alive command is duplicated between the _KEEP_ALIVE_COMMAND constant and Docker provider create(), which can drift over time. Update the create() path in the Docker provider to use _KEEP_ALIVE_COMMAND instead of hardcoding the argv, and keep all keep-alive command construction centralized so the constant is the single source of truth.packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/README.md (1)
1-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMixes explanation and reference content on one page.
Rationale ("Why an owned seam") and reference material (contract/file listing, test listing) sit on the same page. Per coding guidelines, each doc page should fit one Diataxis quadrant with cross-links used instead of mixing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/README.md` around lines 1 - 45, The Sandbox README currently mixes explanatory rationale with reference material, so split it to fit a single Diataxis quadrant. Refactor the content in the Sandbox seam README so the overview and “Why an owned seam” narrative stay separate from the contract, isolation note, roadmap, and tests; move the reference-style file listings and test listings to a dedicated reference page or appendix, and add cross-links from this page to the new target. Keep the main README focused on the conceptual explanation of the sandbox seam and use the existing section titles like “The contract” and “Tests” to relocate or trim the reference details.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/image.py`:
- Around line 82-96: Add bounded timeouts to the Docker subprocess invocations
so they cannot hang indefinitely. In image_exists, wrap the subprocess.run call
with a short timeout and convert timeout expiry into FabricImageError with a
clear daemon-unreachable message; also apply a timeout to the docker build path
in the same fabric image runtime code so both the quick inspect check and build
step fail fast instead of blocking forever.
- Around line 72-158: The cached Fabric image tag only depends on the Dockerfile
and extras, so changes in the staged NeMo-Fabric source can be missed and a
stale image reused. Update fabric_image_tag() so its digest also incorporates
the relevant source contents from the staged fabric_repo (or the same build
inputs used by _stage_source()), and make ensure_fabric_image() compute the tag
after resolving the repo so local source edits produce a new tag and trigger a
rebuild without requiring force_build.
In
`@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/sandbox.Dockerfile`:
- Around line 29-42: The runtime stage in sandbox.Dockerfile is still executing
as root because it never switches away from the default user. Update the runtime
setup after the existing COPY/RUN steps to create and use a dedicated non-root
user, and add a USER instruction so the final image runs sandboxed instead of as
root. Keep the change in the runtime stage near the existing ENV, WORKDIR, and
fabric version check so it is easy to locate alongside the image bootstrap
logic.
---
Nitpick comments:
In
`@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/api.py`:
- Around line 120-127: The `_write_file` helper currently does a local `import
tempfile`, which should be hoisted to the module top level for consistency. Move
the import alongside the other top-level imports in this sandbox API module and
remove the inline import from `_write_file`, keeping the rest of the function
behavior unchanged.
In
`@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/providers/docker.py`:
- Around line 92-121: The _run chokepoint in DockerProvider only handles
timeouts, so a missing docker binary still bubbles up as a raw FileNotFoundError
through create, exec, status, and close. Update _run to catch the
create_subprocess_exec failure for the docker executable and re-raise a clearer,
actionable error that names the missing binary and the argv/context, while
preserving the existing timeout handling and return semantics for successful
runs.
- Line 45: The keep-alive command is duplicated between the _KEEP_ALIVE_COMMAND
constant and Docker provider create(), which can drift over time. Update the
create() path in the Docker provider to use _KEEP_ALIVE_COMMAND instead of
hardcoding the argv, and keep all keep-alive command construction centralized so
the constant is the single source of truth.
In
`@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/README.md`:
- Around line 1-45: The Sandbox README currently mixes explanatory rationale
with reference material, so split it to fit a single Diataxis quadrant. Refactor
the content in the Sandbox seam README so the overview and “Why an owned seam”
narrative stay separate from the contract, isolation note, roadmap, and tests;
move the reference-style file listings and test listings to a dedicated
reference page or appendix, and add cross-links from this page to the new
target. Keep the main README focused on the conceptual explanation of the
sandbox seam and use the existing section titles like “The contract” and “Tests”
to relocate or trim the reference details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4923bf8f-c0ef-495e-8637-c7626bafd2fe
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
packages/nemo_evaluator_sdk/examples/fabric_container/run_e2e.pypackages/nemo_evaluator_sdk/pyproject.tomlpackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/_common.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/image.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/runtime.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/sandbox.Dockerfilepackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/README.mdpackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/api.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/base.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/providers/docker.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_container_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_image.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_integration.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_api.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_docker_provider.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_docker_provider_live.py
Contributor
|
SandyChapman
force-pushed
the
schapman/aalgo-321-containerized-fabric-runner
branch
2 times, most recently
from
c7902c0 to
5382c71
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.py (1)
61-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTYPE_CHECKING-only import of
FabricConfig/FabricProfileConfig.As per coding guidelines, "In Python code, prefer concrete type hints over string-based type hints, and do not import those types only under
TYPE_CHECKING; import them normally when possible." These are imported only underTYPE_CHECKING, relying onfrom __future__ import annotationsfor deferred evaluation. Note_common.pyimportsnemo_relay.observabilitynormally since it's a hard dependency —nemo_fabrichere is optional, which is the likely reason for this pattern, but it still deviates from the stated guideline.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.py` around lines 61 - 64, The `container_runtime.py` module is using `FabricConfig` and `FabricProfileConfig` only inside `TYPE_CHECKING`, which conflicts with the typing guideline. Update the `FabricAgentRuntime`-related imports so these types are imported at runtime when available, and keep the module importable by handling the optional `nemo_fabric` dependency safely instead of relying on `TYPE_CHECKING`-only imports. Use the concrete type names directly in the runtime-facing annotations/config handling paths.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.py`:
- Around line 61-64: The `container_runtime.py` module is using `FabricConfig`
and `FabricProfileConfig` only inside `TYPE_CHECKING`, which conflicts with the
typing guideline. Update the `FabricAgentRuntime`-related imports so these types
are imported at runtime when available, and keep the module importable by
handling the optional `nemo_fabric` dependency safely instead of relying on
`TYPE_CHECKING`-only imports. Use the concrete type names directly in the
runtime-facing annotations/config handling paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 923c56c2-87d8-44ed-90ee-4404ed535c9d
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
packages/nemo_evaluator_sdk/examples/fabric_container/run_e2e.pypackages/nemo_evaluator_sdk/pyproject.tomlpackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/_common.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/image.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/runtime.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/sandbox.Dockerfilepackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/README.mdpackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/api.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/base.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/providers/docker.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_container_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_image.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_integration.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_api.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_docker_provider.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_docker_provider_live.pythird_party/licenses.jsonlthird_party/osv-licenses.jsonthird_party/requirements-main.txttools/nemo-platform-sdk-tools/src/nemo_platform_sdk_tools/license/overrides.yaml
✅ Files skipped from review due to trivial changes (3)
- tools/nemo-platform-sdk-tools/src/nemo_platform_sdk_tools/license/overrides.yaml
- third_party/licenses.jsonl
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/README.md
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/nemo_evaluator_sdk/pyproject.toml
- packages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_image.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_docker_provider.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/base.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/api.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_integration.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_api.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/providers/docker.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/runtime.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_runtime.py
SandyChapman
added a commit
that referenced
this pull request
Jul 8, 2026
The agent-eval runtimes framed the harness prompt with `task.intent` — the eval-side description of the *desired behavior* (what the grader checks for). Exposing it to the agent under evaluation is a reward-hacking hole: the agent can read the grader's intent and target it directly. This mirrors the held-out-`reference` concern (AGENT-EVAL reference field work). Unify the framing so every runtime builds the prompt from `task.inputs` only: - New `runtimes/fabric/_common.py` with a shared, intent-free `fabric_input`; the host `FabricAgentRuntime` now calls it (drops the `Task id/Intent/Inputs` framing). Designed as the shared home so the AALGO-321 container runtime reuses it instead of carrying its own copy. - `default_codex_prompt` no longer emits `Intent:`; it lifts the instruction from `inputs["instruction"|"prompt"]` and keeps its workspace-edit trailer. - `docker_sandbox._task_prompt` no longer falls back to `task.intent`; a task with no instruction in inputs yields an empty prompt rather than leaking it. Tests updated to lock in the intent-free contract across all three runtimes. Relates to AALGO-321 fabric-runtime dedup (PR #604). Signed-off-by: Sandy Chapman <schapman@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.py (1)
61-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTYPE_CHECKING-only import of
FabricConfig/FabricProfileConfig.Guideline forbids gating type imports behind
TYPE_CHECKING. Understand the intent (nemo_fabric is an optional native dep), but this is the exact pattern the guideline disallows — flagging for visibility even though "import normally" here would reintroduce a hard dependency the module deliberately avoids.As per coding guidelines, "do not import those types only under
TYPE_CHECKING; import them normally when possible."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.py` around lines 61 - 64, The `TYPE_CHECKING`-only imports for `FabricConfig` and `FabricProfileConfig` in `container_runtime` violate the import guideline, but the module also needs to stay importable without the optional `nemo_fabric` dependency. Update the typing strategy around `FabricAgentRuntime` and any `to_mapping()`-based config usage so these symbols are available without relying on a `TYPE_CHECKING`-gated import, while still preserving optional-dependency behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.py`:
- Around line 127-153: The cleanup in ContainerRuntime.run_tasks is not
guaranteed because ensure_fabric_image and resolve_secrets run before the
try/finally that closes the provider. Move the image provisioning and secret
resolution into the guarded block in run_tasks so that self._provider.aclose()
still runs if setup fails, keeping the semaphore/gather flow unchanged and
preserving the existing run_one logic.
In
`@packages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_docker_provider_live.py`:
- Around line 26-29: The _docker_ready helper can hang indefinitely on an
unresponsive Docker daemon, blocking the test run instead of skipping cleanly.
Update the subprocess.run call inside _docker_ready to use a bounded timeout and
treat timeout failures the same as an unavailable daemon by returning False.
Keep the change localized to _docker_ready so the live sandbox test setup
continues to use this readiness gate safely.
---
Nitpick comments:
In
`@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.py`:
- Around line 61-64: The `TYPE_CHECKING`-only imports for `FabricConfig` and
`FabricProfileConfig` in `container_runtime` violate the import guideline, but
the module also needs to stay importable without the optional `nemo_fabric`
dependency. Update the typing strategy around `FabricAgentRuntime` and any
`to_mapping()`-based config usage so these symbols are available without relying
on a `TYPE_CHECKING`-gated import, while still preserving optional-dependency
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 849b4043-6962-4af0-b097-d2d035af98a0
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
packages/nemo_evaluator_sdk/examples/fabric_container/run_e2e.pypackages/nemo_evaluator_sdk/pyproject.tomlpackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/_common.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/image.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/runtime.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/sandbox.Dockerfilepackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/README.mdpackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/api.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/base.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/providers/docker.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_container_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_image.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_integration.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_api.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_docker_provider.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_docker_provider_live.pythird_party/licenses.jsonlthird_party/osv-licenses.jsonthird_party/requirements-main.txttools/nemo-platform-sdk-tools/src/nemo_platform_sdk_tools/license/overrides.yaml
✅ Files skipped from review due to trivial changes (1)
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/README.md
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/nemo_evaluator_sdk/pyproject.toml
- tools/nemo-platform-sdk-tools/src/nemo_platform_sdk_tools/license/overrides.yaml
- third_party/licenses.jsonl
- packages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_api.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_runtime.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/base.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/runtime.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_image.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_integration.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/providers/docker.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_docker_provider.py
- third_party/requirements-main.txt
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/api.py
SandyChapman
added a commit
that referenced
this pull request
Jul 8, 2026
The agent-eval runtimes framed the harness prompt with `task.intent` — the eval-side description of the *desired behavior* (what the grader checks for). Exposing it to the agent under evaluation is a reward-hacking hole: the agent can read the grader's intent and target it directly. This mirrors the held-out-`reference` concern (AGENT-EVAL reference field work). Unify the framing so every runtime builds the prompt from `task.inputs` only: - New `runtimes/fabric/_common.py` with a shared, intent-free `fabric_input`; the host `FabricAgentRuntime` now calls it (drops the `Task id/Intent/Inputs` framing). Designed as the shared home so the AALGO-321 container runtime reuses it instead of carrying its own copy. - `default_codex_prompt` no longer emits `Intent:`; it lifts the instruction from `inputs["instruction"|"prompt"]` and keeps its workspace-edit trailer. - `docker_sandbox._task_prompt` no longer falls back to `task.intent`; a task with no instruction in inputs yields an empty prompt rather than leaking it. Tests updated to lock in the intent-free contract across all three runtimes. Relates to AALGO-321 fabric-runtime dedup (PR #604). Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
force-pushed
the
schapman/aalgo-321-containerized-fabric-runner
branch
from
5382c71 to
0945799
Compare
Contributor
Author
|
Thanks @coderabbitai — addressed in the latest push (also rebased onto Fixed
Deferred to a hardening follow-up (tracked; kept out of this spike to preserve the verified live e2e)
|
Contributor
|
Tip For best results, initiate chat on the files or code changes.
|
SandyChapman
force-pushed
the
schapman/aalgo-321-containerized-fabric-runner
branch
from
0945799 to
b037c86
Compare
SandyChapman
force-pushed
the
schapman/aalgo-321-containerized-fabric-runner
branch
from
b037c86 to
7618849
Compare
|
|
||
| # Resolve the model credential into the process env so the default LocalSecretResolver can find it. | ||
| if not os.environ.get("NVIDIA_API_KEY"): | ||
| key_file = os.environ.get("NVIDIA_API_KEY_FILE") |
Contributor
There was a problem hiding this comment.
allowing providing key in a file might be more prone to key leakage compared to env var if user add to the repo and forgets to delete. Do we want to have this option?
Contributor
Author
There was a problem hiding this comment.
Removed the NVIDIA_API_KEY_FILE option from the example; it now reads only from the NVIDIA_API_KEY env var, eliminating the committed-key-file risk.
| status=AgentEvalTrialStatus.COMPLETED, | ||
| output=AgentOutput( | ||
| output_text=_common.extract_output_text(result_payload.get("output")), | ||
| response=result_payload if isinstance(result_payload, dict) else None, |
Contributor
There was a problem hiding this comment.
The host Fabric runtime sets AgentOutput.response to RunResult.output, but this container path stores the entire normalized result envelope. Metrics expecting the shared response shape now see the payload under sample.response.output instead. Should we set this to result_payload.get('output')?
Contributor
Author
There was a problem hiding this comment.
Fixed. Set response = result_payload.get("output") (matching the host runtime's RunResult.output) instead of the whole normalized envelope, so metrics reading sample.response see one shape. Regression test: test_success_response_is_output_payload_not_full_envelope.
| metadata={**base_metadata, "evidence_dir": str(evidence_dir)}, | ||
| ), | ||
| evidence=self._evidence(out_dir, result_path), | ||
| metadata={**base_metadata, "generated": True}, |
Contributor
There was a problem hiding this comment.
Successful container trials omit agent_ok. AgentPhaseSuccessMetric treats a missing value as False, so every successful container run receives a failed phase-success score. Should we match the host Fabric and Codex runtimes by setting agent_ok=True here and False on failed trials?
Contributor
Author
There was a problem hiding this comment.
Fixed. Success trials now stamp agent_ok=True, and the shared build_failed_trial stamps agent_ok=False on failures — matching the host Fabric and Codex runtimes. Regression tests: test_success_trial_stamps_agent_ok_true / test_failed_trial_stamps_agent_ok_false.
| os.killpg(os.getpgid(proc.pid), signal.SIGKILL) | ||
| with contextlib.suppress(Exception): | ||
| await proc.wait() | ||
| raise TimeoutError(f"docker command timed out after {timeout_s:g}s: {argv}") from exc |
Contributor
There was a problem hiding this comment.
reported by codex:
The create argv contains resolved values such as
NVIDIA_API_KEY=..., and this timeout message renders the raw argv.FabricContainerRuntimecatches the resulting error and persists it inerror.json, so a Docker startup timeout leaks credentials into evaluation artifacts. Redact argv before constructing exceptions, using the existing environment-command redaction pattern, and avoid secret values in process arguments where practical.
Contributor
Author
There was a problem hiding this comment.
Fixed. _run now redacts argv via the existing _redact_for_logging before interpolating it into the TimeoutError, so no -e KEY=<secret> reaches error.json. The create/exec paths re-raise that message, so they're covered too. Regression test: test_run_timeout_redacts_secrets_in_argv.
ngoncharenko
approved these changes
Jul 14, 2026
SandyChapman
force-pushed
the
claude/gallant-noether-ac62cb
branch
from
a53beca to
44610da
Compare
SandyChapman
force-pushed
the
schapman/aalgo-321-containerized-fabric-runner
branch
2 times, most recently
from
9fc3839 to
4777ce5
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.py`:
- Around line 275-277: Update the AgentOutput construction in the container
runtime to assign response to the result_payload’s output value, matching
FabricAgentRuntime’s RunResult.output shape, while keeping output_text
extraction unchanged. Ensure container trials expose the response fields
directly rather than the full status/output/error envelope.
- Around line 271-282: Set agent_ok=True in the COMPLETED trial metadata within
container_runtime.py’s container success path, matching host/Codex conventions.
Also update _common.py’s build_failed_trial metadata to set agent_ok=False,
ensuring both successful and failed container trials satisfy
AgentPhaseSuccessMetric’s contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cc703d90-2c67-419a-bb25-44056b1b86b6
⛔ Files ignored due to path filters (8)
sdk/python/nemo-platform/pyproject.tomlis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/fabric/_common.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/fabric/container_runtime.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/fabric/image.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/fabric/sandbox.Dockerfileis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/sandbox/api.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/sandbox/base.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/sandbox/providers/docker.pyis excluded by!sdk/**
📒 Files selected for processing (17)
packages/nemo_evaluator_sdk/examples/fabric_container/run_e2e.pypackages/nemo_evaluator_sdk/pyproject.tomlpackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/_common.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/image.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/sandbox.Dockerfilepackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/README.mdpackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/api.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/base.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/providers/docker.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_container_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_image.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_integration.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_api.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_docker_provider.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_docker_provider_live.pypackages/nemo_platform/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/nemo_evaluator_sdk/pyproject.toml
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/README.md
- packages/nemo_platform/pyproject.toml
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/sandbox.Dockerfile
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/base.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_api.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_integration.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/api.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_docker_provider.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_image.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/providers/docker.py
Add FabricContainerRuntime: runs NeMo Fabric + a coding agent inside a per-task Docker sandbox and maps the result into the existing CandidateEvidence contract, so the workspace-file, run_verifier, and trajectory metrics score container trials with no metric changes. - Provider-neutral sandbox seam (SandboxSpec/SandboxProvider/AsyncSandbox) with a Docker provider that shells out to the docker CLI. Context/artifacts cross the boundary by file transfer (docker cp), so a Kubernetes provider can slot in behind the same seam. AsyncSandbox tears down only its own sandbox; the shared provider's lifetime is the batch owner's (run_tasks disposes it once). - Opaque build-if-missing Fabric image (content-addressed tag) — no Dockerfile for callers to write. - Declare nemo-relay as a hard dependency and build the trajectory profile from its typed config, so Relay owns its schema instead of a hand-maintained dict. - Share the host/container helpers (failed-trial builder, output extraction, evidence subdir naming, trajectory telemetry) in fabric/_common.py so the two runtimes cannot silently drift. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
force-pushed
the
schapman/aalgo-321-containerized-fabric-runner
branch
from
4777ce5 to
7bba384
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds
FabricContainerRuntime(AALGO-321): runs NeMo Fabric + a coding agent inside a per-task Docker sandbox and maps the result into the existingCandidateEvidencecontract, so the workspace-file,run_verifier, and trajectory metrics score container trials with no metric changes.Highlights
SandboxSpec/SandboxProvider/AsyncSandbox) with a Docker provider that shells out to thedockerCLI. Context/artifacts cross the boundary by file transfer (docker cp), so a Kubernetes/agent-sandbox provider can slot in behind the same seam later.AsyncSandboxtears down only its own sandbox; the shared provider's lifetime is the batch owner's (run_tasksdisposes it once).ensure_fabric_imageseam.nemo-relayis now a hard dependency: the trajectory profile is built from Relay's own typed config (via the sharedfabric/_common.py), so Relay owns its schema instead of a hand-maintained dict that silently drifts. Verifiedcp311-abi3wheels exist for every platform the workspace locks.fabric/_common.pyso the two runtimes cannot drift apart.Scope
Exercises the runtime directly (hermes-sdk harness, validated live end-to-end); the plugin
_resolve_targetwiring is a follow-up. Seeexamples/fabric_container/for a runnable end-to-end.Verification
Full
agent_evalsuite green (143 passed, 1 skipped — the gated live Docker test); ruff + pyright clean.Summary by CodeRabbit
New Features
Bug Fixes
Documentation